Conversation
|
5db666e to
1c84ce5
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c84ce5252
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires | ||
| private nativeRum: DdNativeRumType = require('../specs/NativeDdRum') | ||
| .default; | ||
| private nativeRum: DdNativeRumType = getNativeDdRum() as DdNativeRumType; |
There was a problem hiding this comment.
On platforms where a TurboModule lookup is unsupported or throws, imports remain unsafe because DdRum is created through getGlobalInstance during module evaluation and this field immediately calls getNativeDdRum(). The new ./internal entry reaches this wrapper through DdRumResourceTracking -> ResourceReporter -> DdRum, while the public barrel imports it directly; DdLogs, DdTrace, and DdFlags have the same eager field initialization. Resolve the module only when a wrapper operation actually needs it, otherwise both advertised import-safe entry points still perform native lookups.
Useful? React with 👍 / 👎.
| export const getNativeDdSdk = (): Spec | null => { | ||
| if (cachedModule === undefined) { | ||
| cachedModule = TurboModuleRegistry.get<Spec>('DdSdk'); |
There was a problem hiding this comment.
When React Native Codegen processes the configured src/specs directory for a new-architecture build, it expects each TurboModule spec to expose its registry call as the module's default export; moving the only call into this named getter leaves the Spec undiscoverable. The same change was made in all five NativeDd* files, so Codegen can no longer generate the NativeDdSdkSpec, NativeDdRumSpec, and related native base classes consumed by the checked-in Android/iOS implementations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect import safety, codegen compatibility, runtime compatibility, and regression-test validity.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR makes native TurboModule resolution lazy and adds a supported internal entry point for shared core internals.
Changes:
- Adds cached lazy native-module getters.
- Updates SDK wrappers and telemetry integrations.
- Exposes
@datadog/mobile-react-native/internal. - Adds import-safety and internal-entry regression tests.
File summaries
| File | Change |
|---|---|
packages/core/src/trace/DdTrace.ts |
Uses the native trace getter. |
packages/core/src/specs/NativeDdTrace.ts |
Adds lazy trace resolution. |
packages/core/src/specs/NativeDdSdk.ts |
Adds lazy SDK resolution. |
packages/core/src/specs/NativeDdRum.ts |
Adds lazy RUM resolution. |
packages/core/src/specs/NativeDdLogs.ts |
Adds lazy logs resolution. |
packages/core/src/specs/NativeDdFlags.ts |
Adds lazy flags resolution. |
packages/core/src/sdk/DdSdkInternal.ts |
Adds lazy SDK access. |
packages/core/src/sdk/DatadogEventEmitter/DatadogDefaultEventEmitter.tsx |
Uses the SDK getter fallback. |
packages/core/src/rum/instrumentation/interactionTracking/DdBabelInteractionTracking.ts |
Resolves SDK telemetry lazily. |
packages/core/src/rum/instrumentation/interactionTracking/__tests__/DdBabelInteractionTracking.test.ts |
Updates SDK mocking. |
packages/core/src/rum/DdRum.ts |
Uses the native RUM getter. |
packages/core/src/logs/DdLogs.ts |
Uses the native logs getter. |
packages/core/src/internal.ts |
Adds shared internal exports. |
packages/core/src/flags/FlagsClient.ts |
Uses the native flags getter. |
packages/core/src/flags/DdFlags.ts |
Uses the native flags getter. |
packages/core/src/__tests__/internalEntry.test.ts |
Tests internal exports and import safety. |
packages/core/src/__tests__/importSafety.test.ts |
Tests lazy native resolution. |
packages/core/package.json |
Exposes the ./internal subpath. |
Review details
Suppressed comments (1)
packages/core/src/specs/NativeDdFlags.ts:41
- This removes the default export, but
packages/react-native-openfeature/src/__tests__/offlineProvider.integration.test.ts:21still mocksNativeDdFlagswith onlydefault. When the core barrel constructsDdFlagsWrapper,getNativeDdFlagsis undefined and the integration test fails during import. Update that repository mock to provide the named getter and return the stub module.
export const getNativeDdFlags = (): Spec | null => {
- Files reviewed: 18/18 changed files
- Comments generated: 12
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires | ||
| private nativeFlags: DdNativeFlagsType = require('../specs/NativeDdFlags') | ||
| .default; | ||
| private nativeFlags: DdNativeFlagsType = getNativeDdFlags() as DdNativeFlagsType; |
| export { GlobalState } from './sdk/GlobalState/GlobalState'; | ||
| export { BufferSingleton } from './sdk/DatadogProvider/Buffer/BufferSingleton'; | ||
| export { DatadogProviderState } from './sdk/DatadogProvider/DatadogProviderState'; | ||
| export { DdRumResourceTracking } from './rum/instrumentation/resourceTracking/DdRumResourceTracking'; |
| // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires | ||
| private nativeLogs: DdNativeLogsType = require('../specs/NativeDdLogs') | ||
| .default; | ||
| private nativeLogs: DdNativeLogsType = getNativeDdLogs() as DdNativeLogsType; |
| // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires | ||
| private nativeRum: DdNativeRumType = require('../specs/NativeDdRum') | ||
| .default; | ||
| private nativeRum: DdNativeRumType = getNativeDdRum() as DdNativeRumType; |
| // Lazily-backed handle. Resolution happens on first property access (runtime), | ||
| // never at import — so importing this module is safe on platforms without the | ||
| // native module (e.g. Vega). Methods are bound to the resolved module. | ||
| const NativeDdSdk: DdNativeSdkType = new Proxy({} as DdNativeSdkType, { |
| export const getNativeDdRum = (): Spec | null => { | ||
| if (cachedModule === undefined) { | ||
| cachedModule = TurboModuleRegistry.get<Spec>('DdRum'); |
| export const getNativeDdSdk = (): Spec | null => { | ||
| if (cachedModule === undefined) { | ||
| cachedModule = TurboModuleRegistry.get<Spec>('DdSdk'); |
| export const getNativeDdTrace = (): Spec | null => { | ||
| if (cachedModule === undefined) { | ||
| cachedModule = TurboModuleRegistry.get<Spec>('DdTrace'); |
| // eslint-disable-next-line global-require, @typescript-eslint/no-var-requires | ||
| private nativeTrace: DdNativeTraceType = require('../specs/NativeDdTrace') | ||
| .default; | ||
| private nativeTrace: DdNativeTraceType = getNativeDdTrace() as DdNativeTraceType; |
| const getSpy = jest.spyOn(TurboModuleRegistry, 'get'); | ||
| jest.isolateModules(() => { | ||
| require('../internal'); | ||
| }); | ||
| expect(getSpy).not.toHaveBeenCalled(); |
What does this PR do?
This PR makes Datadog React Native native module specs resolve lazily instead of at module importtime. It updates the core SDK wrappers to use explicit native module getter functions, adds regression tests to verify imports no longer trigger
TurboModuleRegistrylookups, and exposes a scoped@datadog/mobile-react-native/internalentry point for platform packages that need shared core internals.Motivation
Some platforms, such as Vega, need to import parts of the core package in environments where the React Native native modules are not available. Resolving TurboModules during import makes those imports fail before the caller actually uses native functionality.
Lazy native module resolution keeps the package import-safe while preserving runtime behavior on supported React Native platforms. The internal entry point also avoids unsupported deep imports from platform packages by providing a single maintained path for shared core internals.
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)